Skip to content

focus_on: check who has the focus after a key that changes nothing visible - #1579

Merged
abrichr merged 5 commits into
mainfrom
feat/focus-on-postcondition
Oct 28, 2026
Merged

abrichr merged 5 commits into
mainfrom
feat/focus-on-postcondition

Conversation

@abrichr

@abrichr abrichr commented Oct 25, 2026

Copy link
Copy Markdown
Member

Bug 22 from the second-installation work (#1548).

Problem

Medplum's form has controls with nothing readable beside them, so #1548 teaches some of its fields by keyboard. Two of those keys change nothing the compiler can see:

  • a Tab onto a small icon button;
  • the ArrowDown that highlights the gender option.

The compiler gave those steps no postcondition, and clinical-write refused the program (prohibit_vacuous_postconditions, max_unverified_steps).

Change

A structural postcondition, focus_on, alongside url_changed and title_changed. It is mined only for key steps that have no visual postcondition.

  • Recorder: around each key or hotkey event, it keeps the focused element's identity (focus_before, focus_after). PlaywrightBackend.focus_identity() reads:
    • the data-testid and the id, each with the count of elements sharing it in the element's document;
    • the role and accessible name from Playwright's accessibility snapshot, with the count of elements sharing that role and name on the page;
    • whether an option is active: aria-activedescendant names an element, or the listbox it controls has an aria-selected option.
  • Compiler: it adds focus_on only when the key moved the focus or made an option active, and the element has a specific identity no other element shared at teach time. That is a testid, then an id, then a role with a non-empty name. A tag alone, a role without a name, or a shared identity gets no postcondition, and the step stays vacuous.
  • Replay fails closed. The focused element must have the recorded identity, and be the only element with it. With focus_active_option, an option must also be active. An unreadable focus, nothing focused, a different element, a second element with the same identity, or a postcondition with no specific identity all fail.
  • Policy: is_vacuous counts a focus_on as asserting something only when it names a specific element (focus_identity_is_specific). A degenerate one is still refused by prohibit_vacuous_postconditions.
  • Other:
    • lint_param_leakage checks the focus name and value for demonstrated parameter values.
    • Durable resume leaves focus_on out of revalidation, as it does local_edit: it is relative to the moment the key was pressed.
    • The new fields are dropped when unset, so existing postconditions and bundle digests serialize exactly as before.

Tests

tests/test_focus_on_postcondition.py (18 tests):

  • Mining: a unique testid, or a unique role with a name, becomes focus_on. A tag only, a role with no name, a shared role and name, a shared testid, or a key that moved nothing gets no postcondition. An option highlighted on the same control is marked focus_active_option.
  • Policy: a specific focus_on makes a key step non-vacuous. One without a specific identity stays vacuous.
  • In real Chromium:
    • passes on the recorded focus;
    • fails on another button with the same role;
    • fails when the name is shared;
    • fails without a specific identity;
    • checks the active option;
    • the recorder keeps the focus around a key.

Local runs:

  • ruff, ruff format, mypy and strict mypy are clean.
  • The postcondition, policy, compile, recorder, local-edit, certification and golden selection gave 739 passed and 2 failed. Both failures (test_record_compile_replay_emit_roundtrip and test_installer_starts_with_an_empty_allow_list_and_default_reboot_policy) fail the same way on a branch without this change: a subprocess can't import the package in this environment.

Not yet run against Medplum. That comes at the next fixture lease, on #1548.

🤖 Generated with Claude Code

…sible

A Tab onto an icon button, or an ArrowDown that highlights an option, gave its step no postcondition, and a strict policy refused it as vacuous. The recorder now keeps the focused element's identity around each key, and the compiler adds a focus_on postcondition when that element has a specific identity no other element shares at teach time: a data-testid, an id, or a role with a non-empty accessible name. At replay the focused element must have that identity and be the only one with it, and, for an option highlight, an option must be active (aria-activedescendant or aria-selected). Anything weaker asserts nothing, never passes, and still counts as vacuous.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abrichr
abrichr marked this pull request as draft October 25, 2026 14:51
@abrichr
abrichr marked this pull request as ready for review October 28, 2026 18:04
@abrichr

abrichr commented Oct 28, 2026

Copy link
Copy Markdown
Member Author

AI review (coordinator)

Verdict: changes requested. Not queued for merge. Three probes in headless Chromium show focus_on passing when focus is on a different element. That makes key steps admissible that prohibit_vacuous_postconditions refused before, even though no single element is identified. CI hasn't run because the PR is a draft (packages-gate: "draft PR: CI runs when marked ready for review").

I reviewed head 3c41e463d, which is up to date with main. The 18 tests in tests/test_focus_on_postcondition.py pass locally against real Chromium. For each probe I recorded focus with focus_identity() at teach time, mined the check with _focus_postcondition, then ran Replayer._focus_on_held on the replay page.

Blocking

  1. An iframe with the same testid gives a vacuous pass. testid_count and id_count count only the focused element's own document, and the postcondition doesn't record the frame chain.
    Page: top <button data-testid=save>Save</button>, plus an iframe containing <button data-testid=save>Other</button>.
    Teach (top button): testid_count: 1, so the check is mined as focus_by=testid, save. The testid isn't unique on the page.
    Replay with focus on the iframe's "Other" button: held=True.
    Fix: store the frame path that _focused_element_call already walks, and require it to match at replay. Or mine testid and id only in the top document, the same rule role and name already follow.

  2. Shadow DOM: the check names the host, not the focused element. document.activeElement resolves to the shadow host, so the identity is the host's.
    Page: <x-w id=w>, whose open shadow root holds buttons "One" and "Delete".
    Teach (focus "One"): mined as focus_by=id, w.
    Replay with focus on "Delete" in the same host: held=True.
    Fix (fail-closed): when el.shadowRoot?.activeElement is set, descend into it, or return unreadable so nothing is mined.

  3. A generated id (React useId) can match a different element. id takes priority over role and name, so :r2: was chosen even though button "Save" was unique.
    Teach page: :r1: Cancel, :r2: Save. Teach (focus Save): mined as focus_by=id, :r2:.
    Replay page, with one extra button rendered first: :r1: Dismiss, :r2: Cancel, :r3: Save.

    • Focus on the wrong element, Cancel (:r2:): held=True.
    • Focus on the right element, Save (:r3:): held=False, a false halt.

    Fix: skip ids that look generated (:r\d+:, «r\d+», _r_\d+_, radix-, headlessui-, mui-\d+, mat-…-\d+, ember\d+, ext-gen) and fall back to role and name. Or store id together with role and name, and require both at replay.

Should fix (rule 3 as agreed)

  1. active_option isn't tied to the controlled listbox. aria-activedescendant passes if the id resolves to any element, and aria-owns is accepted as well as aria-controls. Combobox aria-controls=box1, with the check mined while o1 (in box1) was active:

    • aria-activedescendant pointing at p1, an option in an unrelated box2: held=True.
    • aria-activedescendant pointing at a plain <div id=junk>: held=True.

    Fix: require the target to have role="option" and to sit inside the listbox the control references.

Non-blocking

  1. Race with a script that moves focus. Polling passes on the first frame that matches. I focused "Add Telecom", set a timer to move focus to "Delete all" after 400 ms, and checked at once: held=True. Every polled postcondition has this property, and the next step's own checks still apply, so it isn't blocking. A second read one settle interval later would close it.

Checked and sound

  • Duplicate ids. id_count 2 at teach time falls through to role and name. An id that is unique at teach time but duplicated at replay halts on either copy.
  • Only adds a check. is_vacuous is now stricter: a FOCUS_ON without a specific identity still counts as vacuous, and every other kind behaves as before.
    • The compiler adds the check only to key/hotkey steps whose visual and structural postconditions came back empty.
    • It isn't eligible for the drift-oracle (VLM) rescue.
    • Resume revalidation leaves it out, which is no weaker than before, when those steps had no check.
    • Delivery-uncertainty paths (not step.expect or postconditions_confirmed) become stricter.
    • The one admissibility it newly opens is through findings 1 to 3 above.
  • Fails closed. An unreadable backend, no focus, or an element whose role has no name all fail.

…ope active options

Review fixes. The focused element's frame chain is recorded and must match at replay, so a button with the same testid in an iframe no longer passes. Focus inside a shadow root (or on a custom element that may hide a closed one) is unreadable: no check is mined and any check fails there. A framework-generated id (React useId, Radix, Headless UI, MUI, Ember and similar) is skipped for role and name. An option is active only as a role=option inside the listbox the control names by aria-controls; aria-owns is no longer read. Each hole has a real-Chromium test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abrichr

abrichr commented Oct 28, 2026

Copy link
Copy Markdown
Member Author

AI review (coordinator), re-review of 2447f94f1

Verdict: one blocking hole remains. Not queued for merge. Findings 1, 2 and 4 are fixed. Finding 3, the generated id, is fixed only for the id formats on the list. The current React format still slips through. CI on this head: 6 pass, 3 pending, 1 skipped. The 26 tests in test_focus_on_postcondition.py pass locally against real Chromium.

Re-run of the original probes (headless Chromium)

Case Before Now
A: iframe with the same testid; focus in the iframe held=True held=False (frame_path ["iframe:nth-of-type(1)"] is not ()); top button still True
B: duplicate ids sound sound (falls back to role and name; a duplicate at replay halts)
C: :r2: id shift wrong element True, right element False mined as role_name button "Save": wrong element False, right element True
D: open shadow root host id mined, True on another inner button nothing mined (unreadable)
E: activedescendant to another listbox, to a non-option, aria-owns all True all False
F: script moves focus 400 ms later True True (non-blocking; every polled check has this)

Blocking

G. React 19.2+ useId ids (_r_N_, and _R_N_ from server rendering) aren't on the generated-id list. React 19.2 replaced «r» with _r_ as the default useId format, so most current React apps use it. This is the same case as finding 3:

  • Teach page: _r_1_ Cancel, _r_2_ Save. Teach (focus Save): mined as focus_by=id, _r_2_.
  • Replay page, with one extra button rendered first:
    • Focus on the wrong element, Cancel (now _r_2_): held=True.
    • Focus on the right element, Save (now _r_3_): held=False.

The id list will keep missing formats. These ids are still mined as focus_by=id today: react-aria-:r1: (the :r…: pattern only matches the whole id), mat-input-0 and cdk-overlay-3 (Angular Material and CDK; the numeric suffix is under 4 digits), and rc_select_0 (Ant Design).

Suggested fix: keep the id list, and close the whole class with one structural change. When focus_by is testid or id and the element had a role and a non-empty name at teach time, store them too, and require all of them to match at replay. A shifted id that lands on a different control then fails, because that control's role and name differ. At minimum, add _r_[0-9a-z]+_ and match :r…:, «r…» and _r_…_ anywhere in the id, not only as the whole id.

Non-blocking

  • A closed shadow root on a built-in element still identifies the host. Example: <div id=w tabindex=-1> with attachShadow({mode:'closed'}) holding "One" and "Delete". el.shadowRoot is null for a closed root and div has no hyphen, so the check is mined as id=w and passes with focus on "Delete". This setup is rare. The structural fix above also closes it, because the host div has no role or name.

  • Treating custom elements as unreadable removes checks on common design systems. It fails closed, so it doesn't block. Examples:

    • Angular Material hosts that take focus themselves without any shadow DOM (mat-step-header, mat-chip-option, mat-list-option, mat-tree-node): my mat-step-header role=tab aria-label="Step 2" probe mined nothing.
    • Shoelace, Web Awesome, Material Web, Ionic, Spectrum, Fluent and UI5 buttons with delegatesFocus: sl-button mined nothing.

    A narrower rule would keep those checks: treat the element as unreadable only when it has el.shadowRoot or customElements.get(el.localName) is defined. Angular components aren't registered custom elements, so they would stay readable.

To merge: fix G (ideally the structural fix), then queue.

…er shadow rule

Round 2 of the review. A testid or id check now also records the element's role and accessible name when it had them, and replay requires both, so an id that moves to another element can't pass whatever framework generated it. The generated-id patterns stay as a hint and match anywhere in the id (React's :r…:, «r…» and 19.2's _r_…_; react-aria, Angular Material and CDK, rc-select). Focus is unreadable only on an element with a shadow root or a registered custom element, so unregistered framework components keep their checks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abrichr

abrichr commented Oct 28, 2026

Copy link
Copy Markdown
Member Author

AI review (coordinator), re-review of 9c111ff40

Verdict: no blocking hole left. Queuing auto-merge. CI on this head: 6 pass, 11 pending, 18 skipped, none failed. The 30 tests in test_focus_on_postcondition.py pass locally against real Chromium.

Probes (headless Chromium, all re-run)

Case Result
A: iframe with the same testid; focus in the iframe held=False; the top button True
B: duplicate ids; unique at teach time, duplicated at replay falls back to role and name; a duplicate at replay halts
C: :r2: shift role_name: the wrong element False, the right one True
D: open shadow root nothing mined
E: activedescendant to another listbox, to a non-option, aria-owns all False
F: script moves focus 400 ms later True (non-blocking; every polled check has this)
G: React 19.2 _r_N_ shift role_name: the wrong element False, the right one True
H: mat-input-0, rc_select_0, react-aria-:r1:, cdk-overlay-3, _R_1_ all fall back to role_name
J: Angular-style mat-step-header (unregistered, no shadow) mined as role_name tab "Step 2", so readable again
K: Shoelace-style delegatesFocus button nothing mined (fails closed)
N: registered custom element, no shadow nothing mined (fails closed)

Non-blocking

  • An accessible name that changes at run time now halts. Example: a data-testid=save button named "Save (3)" at teach time and "Save (4)" at replay gives held=False. Badge counts, "Saved 2 min ago" and similar names are affected. This fails closed. If it shows up often, the name check could compare a normalized name, with digits and parenthesized counts removed, when a testid is also bound.

  • An id-only check still remains when the element has no role or name. Binding role and name doesn't cover these cases, and my earlier note that it would close case I was wrong.

    • I (closed shadow root on a plain <div id=w>): mined as id=w; it passes with focus on the inner "Delete" button.
    • M (an unlisted generated id on an unnamed <div tabindex=0>, shifted by one render): passes on the wrong div.

    Both need an unnamed focusable element, and rule 1 allows a bare id. A follow-up could stop creating id-only checks for elements that have no role and name.

  • Inside an iframe, role and name aren't read. An id or testid check there has no role-and-name binding; only the frame path and the id list protect it.

The change only adds a check: is_vacuous stays strict for a FOCUS_ON without a specific identity, nothing else is relaxed, and the VLM rescue can't apply to it.

@abrichr
abrichr merged commit d4b9bc8 into main Oct 28, 2026
37 checks passed
@abrichr
abrichr deleted the feat/focus-on-postcondition branch October 28, 2026 21:34
abrichr added a commit that referenced this pull request Oct 29, 2026
Bumps openadapt-flow to 2.4.1 (pyproject, __version__, the editable root in
uv.lock) and writes the 2.4.1 changelog for everything user-visible merged
since openadapt-flow-v2.4.0: #1593 (Windows config pin), #1594 (product code
no longer imports the benchmark package), #1569 (div, grid and card worklist
rows), #1539 (fault gate per class) and #1579/#1595 (focus_on). Regenerates
llms-full.txt.

Merging this publishes 2.4.1 through release-package.yml once main's
Packages run for this commit passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant